Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: improve assert message #15909

Closed
wants to merge 1 commit into from

Conversation

tnguyen14
Copy link
Contributor

First contribution for Node.js Interactive

Checklist
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the cluster Issues and PRs related to the cluster subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@Trott
Copy link
Member

Trott commented Oct 8, 2017

Hi, @tnguyen14! Welcome and thanks! This change doesn't address the issue that was requested in the Code + Learn task. It still masks that actual values. That is: The message is a string literal that does not provide the contents of the variables that are being compared.

There are two possible ways to address this. One is to remove the message entirely and allow assert.strictEqual() to use its default message which provides the values.

The other way is to change the string literal to a template string and use ${w} and ${cluster.worker} in the template literal to display the values.

Can you update this to use one of those instead?

@tnguyen14
Copy link
Contributor Author

Thanks for the quick response @Trott.

I tried both of the solutions you mentioned. When removing the message completely, the default message is not entirely useful. It would show something like

assert.js:45
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: {} === Worker {
  domain: null,
  _events: {},
  _eventsCount: 0,
  _maxListeners: undefined,
  exitedAfterDisconnect: true,
  state: '
    at Object.<anonymous> (/home/tri/dev/node/test/parallel/test-cluster-worker-destroy.js:54:12)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)

(I made w to {} to trigger the failure)

Using ${w} and ${cluster.worker} doesn't work either, because it just prints out [object Object]. JSON.stringify doesn't work because the object contains circular reference.

I discussed with @mhdawson briefly, and he suggested that it's best to put in an error message that makes the most sense for developers to debug the issue in the case of failure.

Please let me know if you think it's best to drop the error message entirely.

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

I'd be +1 on dropping the error message entirely. AssertionError [ERR_ASSERTION]: {} === Worker { isn't useful by itself, but when you look at the source you'll see assert.strictEqual(w, cluster.worker), which tells you that the assertion failed because w was the empty object not a Worker. I'm not sure printing the contents of the Worker object would be useful in this case.

@Trott
Copy link
Member

Trott commented Oct 10, 2017

I agree with @gibfahn. I think the error message you show when the message argument has been removed entirely is actually quite useful.

@tnguyen14 tnguyen14 force-pushed the improve-test-message branch from a37ea43 to e9618b7 Compare October 10, 2017 22:14
@tnguyen14
Copy link
Contributor Author

Updated the assertion to remove the message.

@Trott
Copy link
Member

Trott commented Oct 11, 2017

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 12, 2017
PR-URL: nodejs#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member

Trott commented Oct 12, 2017

Landed in b6a87db.
Thanks for the contribution! 🎉

@Trott Trott closed this Oct 12, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants